| Message ID | 20240201163324.564525-2-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | VIRTIO-IOMMU: Introduce an aw-bits option | expand |
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: [PATCH v2 1/3] virtio-iommu: Add an option to define the input >range width > >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> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Thanks Zhenzhong > >--- > >v1 -> v2: >- Check the aw-bits value is within [32,64] >--- > include/hw/virtio/virtio-iommu.h | 1 + > hw/virtio/virtio-iommu.c | 7 ++++++- > 2 files changed, 7 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..7870bdbeee 100644 >--- a/hw/virtio/virtio-iommu.c >+++ b/hw/virtio/virtio-iommu.c >@@ -1314,7 +1314,11 @@ 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; >+ if (s->aw_bits < 32 || s->aw_bits > 64) { >+ error_setg(errp, "aw-bits must be within [32,64]"); >+ } >+ s->config.input_range.end = >+ s->aw_bits == 64 ? UINT64_MAX : BIT_ULL(s->aw_bits) - 1; > s->config.domain_range.end = UINT32_MAX; > s->config.probe_size = VIOMMU_PROBE_SIZE; > >@@ -1525,6 +1529,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(), > }; > >-- >2.41.0
On 2/1/24 17:32, Eric Auger 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> > > --- > > v1 -> v2: > - Check the aw-bits value is within [32,64] > --- > include/hw/virtio/virtio-iommu.h | 1 + > hw/virtio/virtio-iommu.c | 7 ++++++- > 2 files changed, 7 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..7870bdbeee 100644 > --- a/hw/virtio/virtio-iommu.c > +++ b/hw/virtio/virtio-iommu.c > @@ -1314,7 +1314,11 @@ 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; > + if (s->aw_bits < 32 || s->aw_bits > 64) { > + error_setg(errp, "aw-bits must be within [32,64]"); > + } > + s->config.input_range.end = > + s->aw_bits == 64 ? UINT64_MAX : BIT_ULL(s->aw_bits) - 1; This could be simplified : s->config.input_range.end = BIT_ULL(s->aw_bits) - 1; Anyhow, Reviewed-by: Cédric Le Goater <clg@redhat.com> Thanks, C. > s->config.domain_range.end = UINT32_MAX; > s->config.probe_size = VIOMMU_PROBE_SIZE; > > @@ -1525,6 +1529,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(), > }; >
Hi Eric, On Thu, Feb 01, 2024 at 05:32:22PM +0100, Eric Auger 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> > > --- > > v1 -> v2: > - Check the aw-bits value is within [32,64] > --- > include/hw/virtio/virtio-iommu.h | 1 + > hw/virtio/virtio-iommu.c | 7 ++++++- > 2 files changed, 7 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..7870bdbeee 100644 > --- a/hw/virtio/virtio-iommu.c > +++ b/hw/virtio/virtio-iommu.c > @@ -1314,7 +1314,11 @@ 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; > + if (s->aw_bits < 32 || s->aw_bits > 64) { I'm wondering if we should lower this to 16 bits, just to support all possible host SMMU configurations (the smallest address space configurable with T0SZ is 25-bit, or 16-bit with the STT extension). Thanks, Jean > + error_setg(errp, "aw-bits must be within [32,64]"); > + } > + s->config.input_range.end = > + s->aw_bits == 64 ? UINT64_MAX : BIT_ULL(s->aw_bits) - 1; > s->config.domain_range.end = UINT32_MAX; > s->config.probe_size = VIOMMU_PROBE_SIZE; > > @@ -1525,6 +1529,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(), > }; > > -- > 2.41.0 >
On 2/5/24 10:14, Cédric Le Goater wrote: > On 2/1/24 17:32, Eric Auger 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> >> >> --- >> >> v1 -> v2: >> - Check the aw-bits value is within [32,64] >> --- >> include/hw/virtio/virtio-iommu.h | 1 + >> hw/virtio/virtio-iommu.c | 7 ++++++- >> 2 files changed, 7 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..7870bdbeee 100644 >> --- a/hw/virtio/virtio-iommu.c >> +++ b/hw/virtio/virtio-iommu.c >> @@ -1314,7 +1314,11 @@ 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; >> + if (s->aw_bits < 32 || s->aw_bits > 64) { >> + error_setg(errp, "aw-bits must be within [32,64]"); >> + } >> + s->config.input_range.end = >> + s->aw_bits == 64 ? UINT64_MAX : BIT_ULL(s->aw_bits) - 1; > > > This could be simplified : > > s->config.input_range.end = BIT_ULL(s->aw_bits) - 1; Forget that. We would need a int28. Thanks, C. > > Anyhow, > > > Reviewed-by: Cédric Le Goater <clg@redhat.com> > > Thanks, > > C. > > > >> s->config.domain_range.end = UINT32_MAX; >> s->config.probe_size = VIOMMU_PROBE_SIZE; >> @@ -1525,6 +1529,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(), >> }; >
Hi Jean, On 2/5/24 11:13, Jean-Philippe Brucker wrote: > Hi Eric, > > On Thu, Feb 01, 2024 at 05:32:22PM +0100, Eric Auger 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> >> >> --- >> >> v1 -> v2: >> - Check the aw-bits value is within [32,64] >> --- >> include/hw/virtio/virtio-iommu.h | 1 + >> hw/virtio/virtio-iommu.c | 7 ++++++- >> 2 files changed, 7 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..7870bdbeee 100644 >> --- a/hw/virtio/virtio-iommu.c >> +++ b/hw/virtio/virtio-iommu.c >> @@ -1314,7 +1314,11 @@ 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; >> + if (s->aw_bits < 32 || s->aw_bits > 64) { > I'm wondering if we should lower this to 16 bits, just to support all > possible host SMMU configurations (the smallest address space configurable > with T0SZ is 25-bit, or 16-bit with the STT extension). I have no objection relaxing that check. Alex suggested this 32b low limit with his knowledge of x86. I am a bit reluctant to add extra machine specific checks though. Anyway I assume that some [low] aw-bits will fail with some guests and this will be difficult to understand from bug reports. Thanks Eric > > Thanks, > Jean > >> + error_setg(errp, "aw-bits must be within [32,64]"); >> + } >> + s->config.input_range.end = >> + s->aw_bits == 64 ? UINT64_MAX : BIT_ULL(s->aw_bits) - 1; >> s->config.domain_range.end = UINT32_MAX; >> s->config.probe_size = VIOMMU_PROBE_SIZE; >> >> @@ -1525,6 +1529,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(), >> }; >> >> -- >> 2.41.0 >>
Hi Jean, On 2/5/24 11:13, Jean-Philippe Brucker wrote: > Hi Eric, > > On Thu, Feb 01, 2024 at 05:32:22PM +0100, Eric Auger 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> >> >> --- >> >> v1 -> v2: >> - Check the aw-bits value is within [32,64] >> --- >> include/hw/virtio/virtio-iommu.h | 1 + >> hw/virtio/virtio-iommu.c | 7 ++++++- >> 2 files changed, 7 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..7870bdbeee 100644 >> --- a/hw/virtio/virtio-iommu.c >> +++ b/hw/virtio/virtio-iommu.c >> @@ -1314,7 +1314,11 @@ 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; >> + if (s->aw_bits < 32 || s->aw_bits > 64) { > I'm wondering if we should lower this to 16 bits, just to support all > possible host SMMU configurations (the smallest address space configurable > with T0SZ is 25-bit, or 16-bit with the STT extension). Is it a valid use case case to assign host devices protected by virtio-iommu with a physical SMMU featuring Small Translation Table? It leaves 64kB IOVA space only. Besides in the spec, it is wriiten the min T0SZ can even be 12. "The minimum valid value is 16 unless all of the following also hold, in which case the minimum permitted value is 12: – SMMUv3.1 or later is supported. – SMMU_IDR5.VAX indicates support for 52-bit Vas. – The corresponding CD.TGx selects a 64KB granule. " At the moment I would prefer to stick to the limit suggested by Alex which looks also sensible for other archs whereas 16 doesn't. Thanks Eric > > Thanks, > Jean > >> + error_setg(errp, "aw-bits must be within [32,64]"); >> + } >> + s->config.input_range.end = >> + s->aw_bits == 64 ? UINT64_MAX : BIT_ULL(s->aw_bits) - 1; >> s->config.domain_range.end = UINT32_MAX; >> s->config.probe_size = VIOMMU_PROBE_SIZE; >> >> @@ -1525,6 +1529,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(), >> }; >> >> -- >> 2.41.0 >>
On Thu, Feb 08, 2024 at 09:16:35AM +0100, Eric Auger wrote: > >> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c > >> index ec2ba11d1d..7870bdbeee 100644 > >> --- a/hw/virtio/virtio-iommu.c > >> +++ b/hw/virtio/virtio-iommu.c > >> @@ -1314,7 +1314,11 @@ 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; > >> + if (s->aw_bits < 32 || s->aw_bits > 64) { > > I'm wondering if we should lower this to 16 bits, just to support all > > possible host SMMU configurations (the smallest address space configurable > > with T0SZ is 25-bit, or 16-bit with the STT extension). > Is it a valid use case case to assign host devices protected by > virtio-iommu with a physical SMMU featuring Small Translation Table? Probably not, I'm guessing STT is for tiny embedded implementations where QEMU or even virtualization is not a use case. But because smaller mobile platforms now implement SMMUv3, using smaller IOVA spaces and thus fewer page tables can be beneficial. One use case I have in mind is android with pKVM, each app has its own VM, and devices can be partitioned into lots of address spaces with PASID, so you can save a lot of memory and table-walk time by shrinking those address space. But that particular case will use crosvm so isn't relevant here, it's only an example. Mainly I was concerned that if the Linux driver decides to allow configuring smaller address spaces (maybe a linux cmdline option), then using a architectural limit here would be a safe bet that things can still work. But we can always change it in a later version, or implement finer controls (ideally the guest driver would configure the VA size in ATTACH). > It leaves 64kB IOVA space only. Besides in the spec, it is wriiten the > min T0SZ can even be 12. > > "The minimum valid value is 16 unless all of the following also hold, in > which case the minimum permitted > value is 12: > – SMMUv3.1 or later is supported. > – SMMU_IDR5.VAX indicates support for 52-bit Vas. > – The corresponding CD.TGx selects a 64KB granule. > " Yes that's confusing because va_size = 64 - T0SZ, so T0SZ=12 actually describes the largest address size, 52. > > At the moment I would prefer to stick to the limit suggested by Alex > which looks also sensible for other archs whereas 16 doesn't. Agreed, it should be sufficient. Thanks, Jean
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..7870bdbeee 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -1314,7 +1314,11 @@ 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; + if (s->aw_bits < 32 || s->aw_bits > 64) { + error_setg(errp, "aw-bits must be within [32,64]"); + } + s->config.input_range.end = + s->aw_bits == 64 ? UINT64_MAX : BIT_ULL(s->aw_bits) - 1; s->config.domain_range.end = UINT32_MAX; s->config.probe_size = VIOMMU_PROBE_SIZE; @@ -1525,6 +1529,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(), };
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> --- v1 -> v2: - Check the aw-bits value is within [32,64] --- include/hw/virtio/virtio-iommu.h | 1 + hw/virtio/virtio-iommu.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)