Re: [PATCH 1/3] virtio-iommu: Add an option to define the input range width

[Alex Williamson]

On Wed, 24 Jan 2024 14:57:41 +0100
Eric Auger <eric.auger@redhat.com> wrote:

> Hi Alex,
> 
> On 1/24/24 14:37, Alex Williamson wrote:
> > On Wed, 24 Jan 2024 14:14:19 +0100
> > Eric Auger <eric.auger@redhat.com> wrote:
> >  
> >> Hi Alex,
> >>
> >> On 1/24/24 00:51, Alex Williamson wrote:  
> >>> On Tue, 23 Jan 2024 19:15:55 +0100
> >>> Eric Auger <eric.auger@redhat.com> wrote:
> >>>    
> >>>> aw-bits is a new option that allows to set the bit width of
> >>>> the input address range. This value will be used as a default for
> >>>> the device config input_range.end. By default it is set to 64 bits
> >>>> which is the current value.
> >>>>
> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>>> ---
> >>>>  include/hw/virtio/virtio-iommu.h | 1 +
> >>>>  hw/virtio/virtio-iommu.c         | 4 +++-
> >>>>  2 files changed, 4 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/include/hw/virtio/virtio-iommu.h 
> >>>> b/include/hw/virtio/virtio-iommu.h
> >>>> index 781ebaea8f..5fbe4677c2 100644
> >>>> --- a/include/hw/virtio/virtio-iommu.h
> >>>> +++ b/include/hw/virtio/virtio-iommu.h
> >>>> @@ -66,6 +66,7 @@ struct VirtIOIOMMU {
> >>>>      bool boot_bypass;
> >>>>      Notifier machine_done;
> >>>>      bool granule_frozen;
> >>>> +    uint8_t aw_bits;
> >>>>  };
> >>>>  
> >>>>  #endif
> >>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> >>>> index ec2ba11d1d..e7f299e0c6 100644
> >>>> --- a/hw/virtio/virtio-iommu.c
> >>>> +++ b/hw/virtio/virtio-iommu.c
> >>>> @@ -1314,7 +1314,8 @@ static void 
> >>>> virtio_iommu_device_realize(DeviceState *dev, Error **errp)
> >>>>       */
> >>>>      s->config.bypass = s->boot_bypass;
> >>>>      s->config.page_size_mask = qemu_real_host_page_mask();
> >>>> -    s->config.input_range.end = UINT64_MAX;
> >>>> +    s->config.input_range.end =
> >>>> +        s->aw_bits == 64 ? UINT64_MAX : BIT_ULL(s->aw_bits) - 1;    
> >>> What happens when someone sets aw_bits = 1?  There are a whole bunch of
> >>> impractical values here ripe for annoying bug reports.  vtd_realize()
> >>> returns an Error for any values other than 39 or 48.  We might pick an
> >>> arbitrary lower bound (39?) or some other more creative solution here
> >>> to avoid those silly issues in our future.  Thanks,    
> >> You're right. I can check the input value. This needs to be dependent on
> >> the machine though but this should be feasable.
> >> Then I would allow 39 and 48 for q35 and 64 only on ARM.  
> > AFAIK AMD-Vi supports 64-bit address space.  Without querying the host
> > there's no way to place an accurate limit below 64-bit.  Thanks,  
> 
> Hum this means I would need to look at
> /sys/class/iommu/<iommu>/amd-iommu/ or /sys/class/iommu/dmar* to
> discriminate between AMD IOMMU and INTEL IOMMU physical IOMMU. Would
> that be acceptable?

I'm not necessarily suggesting that we look at the host, I'm mostly
just stating that enforcing 39/48 bits on non-ARM is incorrect for a
large portion of the non-ARM world too.  There might even be some
interesting use cases for a 32-bit IOVA space, so maybe just set
defaults tuned for compatibility and accept anything between 32- and
64-bits.  Thanks,

Alex

> >>>>      s->config.domain_range.end = UINT32_MAX;
> >>>>      s->config.probe_size = VIOMMU_PROBE_SIZE;
> >>>>  
> >>>> @@ -1525,6 +1526,7 @@ static Property virtio_iommu_properties[] = {
> >>>>      DEFINE_PROP_LINK("primary-bus", VirtIOIOMMU, primary_bus,
> >>>>                       TYPE_PCI_BUS, PCIBus *),
> >>>>      DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
> >>>> +    DEFINE_PROP_UINT8("aw-bits", VirtIOIOMMU, aw_bits, 64),
> >>>>      DEFINE_PROP_END_OF_LIST(),
> >>>>  };
> >>>>      
>