Re: [Qemu-devel] [PATCH v3 1/2] intel-iommu: differentiate host address width from IOVA address width.

[Yu Zhang]

On Fri, Dec 28, 2018 at 11:29:41PM -0200, Eduardo Habkost wrote:
> On Fri, Dec 28, 2018 at 10:32:59AM +0800, Yu Zhang wrote:
> > On Thu, Dec 27, 2018 at 01:14:11PM -0200, Eduardo Habkost wrote:
> > > On Wed, Dec 26, 2018 at 01:30:00PM +0800, Yu Zhang wrote:
> > > > On Tue, Dec 25, 2018 at 11:56:19AM -0500, Michael S. Tsirkin wrote:
> > > > > On Sat, Dec 22, 2018 at 09:11:26AM +0800, Yu Zhang wrote:
> > > > > > On Fri, Dec 21, 2018 at 02:02:28PM -0500, Michael S. Tsirkin wrote:
> > > > > > > On Sat, Dec 22, 2018 at 01:37:58AM +0800, Yu Zhang wrote:
> > > > > > > > On Fri, Dec 21, 2018 at 12:04:49PM -0500, Michael S. Tsirkin 
> > > > > > > > wrote:
> > > > > > > > > On Sat, Dec 22, 2018 at 12:09:44AM +0800, Yu Zhang wrote:
> > > > > > > > > > Well, my understanding of the vt-d spec is that the address 
> > > > > > > > > > limitation in
> > > > > > > > > > DMAR are referring to the same concept of 
> > > > > > > > > > CPUID.MAXPHYSADDR. I do not think
> > > > > > > > > > there's any different in the native scenario. :)
> > > > > > > > > 
> > > > > > > > > I think native machines exist on which the two values are 
> > > > > > > > > different.
> > > > > > > > > Is that true?
> > > > > > > > 
> > > > > > > > I think the answer is not. My understanding is that HAW(host 
> > > > > > > > address wdith) is
> > > > > > > > the maximum physical address width a CPU can detects(by 
> > > > > > > > cpuid.0x80000008).
> > > > > > > > 
> > > > > > > > I agree there are some addresses the CPU does not touch, but 
> > > > > > > > they are still in
> > > > > > > > the physical address space, and there's only one physical 
> > > > > > > > address space...
> > > > > > > > 
> > > > > > > > B.R.
> > > > > > > > Yu
> > > > > > > 
> > > > > > > Ouch I thought we are talking about the virtual address size.
> > > > > > > I think I did have a box where VTD's virtual address size was
> > > > > > > smaller than CPU's.
> > > > > > > For physical one - we just need to make it as big as max supported
> > > > > > > memory right?
> > > > > > 
> > > > > > Well, my understanding of the physical one is the maximum physical 
> > > > > > address
> > > > > > width. Sorry, this explain seems nonsense... I mean, it's not just 
> > > > > > about
> > > > > > the max supported memory, but also covers MMIO. It shall be 
> > > > > > detectable
> > > > > > from cpuid, or ACPI's DMAR table, instead of calculated by the max 
> > > > > > memory
> > > > > > size. One common usage of this value is to tell the paging 
> > > > > > structure entries(
> > > > > > CPU's or IOMMU's) which bits shall be reserved. There are also some 
> > > > > > registers
> > > > > > e.g. apic base reg etc, whose contents are physical addresses, 
> > > > > > therefore also
> > > > > > need to follow the similar requirement for the reserved bits.
> > > > > > 
> > > > > > So I think the correct direction might be to define this property 
> > > > > > in the
> > > > > > machine status level, instead of the CPU level. Is this reasonable 
> > > > > > to you?
> > > > > 
> > > > > At that level yes. But isn't this already specified by 
> > > > > "pci-hole64-end"?
> > > > 
> > > > But this value is set by guest firmware? Will PCI hotplug change this 
> > > > address?
> > > > 
> > > > @Eduardo, do you have any plan to calculate the phys-bits by 
> > > > "pci-hole64-end"?
> > > > Or introduce another property, say "max-phys-bits" in machine status?
> > > 
> > > I agree it may make sense to make the machine code control
> > > phys-bits instead of the CPU object.  A machine property sounds
> > > like the simplest solution.
> > > 
> > > But I don't think we can have a meaningful discussion about
> > > implementation if we don't agree about the command-line
> > > interface.  We must decide what will happen to the CPU and iommu
> > > physical address width in cases like:
> > 
> > Thanks, Eduardo.
> > 
> > What about we just use "-machine phys-bits=52", and remove the
> > "phys-bits" from CPU parameter?
> 
> Maybe we can deprecate it, but we can't remove it immediately.
> We still need to decide what to do on the cases below, while the
> option is still available.

I saw the ACPI DMAR is ininitialized in acpi_build(), which is called
by pc_machine_done(). I guess this is done after the initialization of
vCPU and vIOMMU.

So I am wondering, instead of moving "phys-bits" from X86CPU into the
MachineState, maybe we could:

1> Define a "phys_bits" in MachineState or PCMachineState(not sure which
one is more suitable).

2> Set ms->phys_bits in x86_cpu_realizefn().

3> Since DMAR is created after vCPU creation, we can build DMAR table
with ms->phys_bits.

4> Also, we can reset the hardware address width for vIOMMU(and the
vtd_paging_entry_rsvd_field array) in pc_machine_done(), based on the value
of ms->phys_bits, or from ACPI DMAR table(from spec point of view, address
width limitation of IOMMU shall come from DMAR, yet I have not figured out
any simple approach to probe the ACPI property). 

This way, we do not need worry about the initialization sequence of vCPU
and vIOMMU, and both DMAR and IOMMU setting are from the machine level which
follows the spec.

Any comments? :)

B.R.
Yu

> 
> > 
> > > 
> > >   $QEMU -device intel-iommu
> > >   $QEMU -cpu ...,phys-bits=50 -device intel-iommu
> > >   $QEMU -cpu ...,host-phys-bits=on -device intel-iommu
> > >   $QEMU -machine phys-bits=50 -device intel-iommu
> > >   $QEMU -machine phys-bits=50 -cpu ...,phys-bits=48 -device intel-iommu
> > > 
> > > -- 
> > > Eduardo
> > > 
> > 
> > B.R.
> > Yu
> 
> -- 
> Eduardo
>