| Message ID | 20240123181753.413961-2-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | VIRTIO-IOMMU: Introduce an aw-bits option | expand |
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, 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(), > }; >
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. Eric > > 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(), >> }; >>
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, 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(), > >> }; > >> >
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? Eric > > 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(), >>>> }; >>>>
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(), > >>>> }; > >>>> >
Hi Alex, On 1/24/24 15:15, Alex Williamson wrote: > 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, Yup that would be even simpler. Thank you for the clarification. I will add that Eric > > 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(), >>>>>> }; >>>>>>
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; 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(), };
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(-)