| Message ID | 20240626082727.1278530-6-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | VIRTIO-IOMMU/HostIOMMUDevice: Fixes and page size mask rework | expand |
Hi Eric, >-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: [PATCH 5/7] virtio-iommu : Retrieve page size mask on >virtio_iommu_set_iommu_device() > >Retrieve the Host IOMMU Device page size mask when this latter is set. >This allows to get the information much sooner than when relying on >IOMMU MR set_page_size_mask() call, whcih happens when the IOMMU >MR >gets enabled. We introduce check_page_size_mask() helper whose code >is inherited from current virtio_iommu_set_page_size_mask() >implementation. This callback will be removed in a subsequent patch. > >Signed-off-by: Eric Auger <eric.auger@redhat.com> >--- > hw/virtio/virtio-iommu.c | 55 >++++++++++++++++++++++++++++++++++++++-- > hw/virtio/trace-events | 1 + > 2 files changed, 54 insertions(+), 2 deletions(-) > >diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c >index b8f75d2b1a..631589735a 100644 >--- a/hw/virtio/virtio-iommu.c >+++ b/hw/virtio/virtio-iommu.c >@@ -598,9 +598,39 @@ out: > return ret; > } > >+static bool check_page_size_mask(VirtIOIOMMU *viommu, uint64_t >new_mask, >+ Error **errp) >+{ >+ uint64_t cur_mask = viommu->config.page_size_mask; >+ >+ if ((cur_mask & new_mask) == 0) { >+ error_setg(errp, "virtio-iommu reports a page size mask 0x%"PRIx64 >+ " incompatible with currently supported mask 0x%"PRIx64, >+ new_mask, cur_mask); >+ return false; >+ } >+ /* >+ * Once the granule is frozen we can't change the mask anymore. If by >+ * chance the hotplugged device supports the same granule, we can still >+ * accept it. >+ */ >+ if (viommu->granule_frozen) { >+ int cur_granule = ctz64(cur_mask); >+ >+ if (!(BIT_ULL(cur_granule) & new_mask)) { >+ error_setg(errp, >+ "virtio-iommu does not support frozen granule 0x%llx", >+ BIT_ULL(cur_granule)); >+ return false; >+ } >+ } >+ return true; >+} >+ > static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, >int devfn, > HostIOMMUDevice *hiod, Error **errp) > { >+ ERRP_GUARD(); > VirtIOIOMMU *viommu = opaque; > HostIOMMUDeviceClass *hiodc = >HOST_IOMMU_DEVICE_GET_CLASS(hiod); > struct hiod_key *new_key; >@@ -623,8 +653,26 @@ static bool >virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn, > hiod->aliased_devfn, > host_iova_ranges, errp); > if (ret) { >- g_list_free_full(host_iova_ranges, g_free); >- return false; >+ goto error; >+ } >+ } >+ if (hiodc->get_page_size_mask) { >+ uint64_t new_mask = hiodc->get_page_size_mask(hiod); >+ >+ if (check_page_size_mask(viommu, new_mask, errp)) { >+ /* >+ * The default mask depends on the "granule" property. For example, >+ * with 4k granule, it is -(4 * KiB). When an assigned device has >+ * page size restrictions due to the hardware IOMMU configuration, >+ * apply this restriction to the mask. >+ */ >+ trace_virtio_iommu_update_page_size_mask(hiod->name, >+ viommu->config.page_size_mask, >+ new_mask); >+ viommu->config.page_size_mask &= new_mask; This is a bit different from original logic, it may update page_size_mask after frozen. Will that make issue? Except this question, for all other patches, Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Thanks Zhenzhong >+ } else { >+ error_prepend(errp, "%s: ", hiod->name); >+ goto error; > } > } > >@@ -637,6 +685,9 @@ static bool >virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn, > g_list_free_full(host_iova_ranges, g_free); > > return true; >+error: >+ g_list_free_full(host_iova_ranges, g_free); >+ return false; > } > > static void >diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events >index 3cf84e04a7..599d855ff6 100644 >--- a/hw/virtio/trace-events >+++ b/hw/virtio/trace-events >@@ -132,6 +132,7 @@ virtio_iommu_notify_map(const char *name, >uint64_t virt_start, uint64_t virt_end > virtio_iommu_notify_unmap(const char *name, uint64_t virt_start, >uint64_t virt_end) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64 > virtio_iommu_remap(const char *name, uint64_t virt_start, uint64_t >virt_end, uint64_t phys_start) "mr=%s virt_start=0x%"PRIx64" >virt_end=0x%"PRIx64" phys_start=0x%"PRIx64 > virtio_iommu_set_page_size_mask(const char *name, uint64_t old, >uint64_t new) "mr=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64 >+virtio_iommu_update_page_size_mask(const char *name, uint64_t old, >uint64_t new) "host iommu device=%s old_mask=0x%"PRIx64" >new_mask=0x%"PRIx64 > virtio_iommu_notify_flag_add(const char *name) "add notifier to mr %s" > virtio_iommu_notify_flag_del(const char *name) "del notifier from mr %s" > virtio_iommu_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn, >bool on) "Device %02x:%02x.%x switching address space (iommu >enabled=%d)" >-- >2.41.0
Hi Zhenhong, On 6/27/24 13:32, Duan, Zhenzhong wrote: > Hi Eric, > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Subject: [PATCH 5/7] virtio-iommu : Retrieve page size mask on >> virtio_iommu_set_iommu_device() >> >> Retrieve the Host IOMMU Device page size mask when this latter is set. >> This allows to get the information much sooner than when relying on >> IOMMU MR set_page_size_mask() call, whcih happens when the IOMMU >> MR >> gets enabled. We introduce check_page_size_mask() helper whose code >> is inherited from current virtio_iommu_set_page_size_mask() >> implementation. This callback will be removed in a subsequent patch. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> --- >> hw/virtio/virtio-iommu.c | 55 >> ++++++++++++++++++++++++++++++++++++++-- >> hw/virtio/trace-events | 1 + >> 2 files changed, 54 insertions(+), 2 deletions(-) >> >> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c >> index b8f75d2b1a..631589735a 100644 >> --- a/hw/virtio/virtio-iommu.c >> +++ b/hw/virtio/virtio-iommu.c >> @@ -598,9 +598,39 @@ out: >> return ret; >> } >> >> +static bool check_page_size_mask(VirtIOIOMMU *viommu, uint64_t >> new_mask, >> + Error **errp) >> +{ >> + uint64_t cur_mask = viommu->config.page_size_mask; >> + >> + if ((cur_mask & new_mask) == 0) { >> + error_setg(errp, "virtio-iommu reports a page size mask 0x%"PRIx64 >> + " incompatible with currently supported mask 0x%"PRIx64, >> + new_mask, cur_mask); >> + return false; >> + } >> + /* >> + * Once the granule is frozen we can't change the mask anymore. If by >> + * chance the hotplugged device supports the same granule, we can still >> + * accept it. >> + */ >> + if (viommu->granule_frozen) { >> + int cur_granule = ctz64(cur_mask); >> + >> + if (!(BIT_ULL(cur_granule) & new_mask)) { >> + error_setg(errp, >> + "virtio-iommu does not support frozen granule 0x%llx", >> + BIT_ULL(cur_granule)); >> + return false; >> + } >> + } >> + return true; >> +} >> + >> static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, >> int devfn, >> HostIOMMUDevice *hiod, Error **errp) >> { >> + ERRP_GUARD(); >> VirtIOIOMMU *viommu = opaque; >> HostIOMMUDeviceClass *hiodc = >> HOST_IOMMU_DEVICE_GET_CLASS(hiod); >> struct hiod_key *new_key; >> @@ -623,8 +653,26 @@ static bool >> virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn, >> hiod->aliased_devfn, >> host_iova_ranges, errp); >> if (ret) { >> - g_list_free_full(host_iova_ranges, g_free); >> - return false; >> + goto error; >> + } >> + } >> + if (hiodc->get_page_size_mask) { >> + uint64_t new_mask = hiodc->get_page_size_mask(hiod); >> + >> + if (check_page_size_mask(viommu, new_mask, errp)) { >> + /* >> + * The default mask depends on the "granule" property. For example, >> + * with 4k granule, it is -(4 * KiB). When an assigned device has >> + * page size restrictions due to the hardware IOMMU configuration, >> + * apply this restriction to the mask. >> + */ >> + trace_virtio_iommu_update_page_size_mask(hiod->name, >> + viommu->config.page_size_mask, >> + new_mask); >> + viommu->config.page_size_mask &= new_mask; > This is a bit different from original logic, it may update page_size_mask after frozen. > Will that make issue? You're fully right. I need to replace viommu->config.page_size_mask &= new_mask; by if (s->granule_frozen) { viommu->config.page_size_mask &= new_mask; } > Except this question, for all other patches, > > Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Thanks! Eric > > Thanks > Zhenzhong > >> + } else { >> + error_prepend(errp, "%s: ", hiod->name); >> + goto error; >> } >> } >> >> @@ -637,6 +685,9 @@ static bool >> virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn, >> g_list_free_full(host_iova_ranges, g_free); >> >> return true; >> +error: >> + g_list_free_full(host_iova_ranges, g_free); >> + return false; >> } >> >> static void >> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events >> index 3cf84e04a7..599d855ff6 100644 >> --- a/hw/virtio/trace-events >> +++ b/hw/virtio/trace-events >> @@ -132,6 +132,7 @@ virtio_iommu_notify_map(const char *name, >> uint64_t virt_start, uint64_t virt_end >> virtio_iommu_notify_unmap(const char *name, uint64_t virt_start, >> uint64_t virt_end) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64 >> virtio_iommu_remap(const char *name, uint64_t virt_start, uint64_t >> virt_end, uint64_t phys_start) "mr=%s virt_start=0x%"PRIx64" >> virt_end=0x%"PRIx64" phys_start=0x%"PRIx64 >> virtio_iommu_set_page_size_mask(const char *name, uint64_t old, >> uint64_t new) "mr=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64 >> +virtio_iommu_update_page_size_mask(const char *name, uint64_t old, >> uint64_t new) "host iommu device=%s old_mask=0x%"PRIx64" >> new_mask=0x%"PRIx64 >> virtio_iommu_notify_flag_add(const char *name) "add notifier to mr %s" >> virtio_iommu_notify_flag_del(const char *name) "del notifier from mr %s" >> virtio_iommu_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn, >> bool on) "Device %02x:%02x.%x switching address space (iommu >> enabled=%d)" >> -- >> 2.41.0
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index b8f75d2b1a..631589735a 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -598,9 +598,39 @@ out: return ret; } +static bool check_page_size_mask(VirtIOIOMMU *viommu, uint64_t new_mask, + Error **errp) +{ + uint64_t cur_mask = viommu->config.page_size_mask; + + if ((cur_mask & new_mask) == 0) { + error_setg(errp, "virtio-iommu reports a page size mask 0x%"PRIx64 + " incompatible with currently supported mask 0x%"PRIx64, + new_mask, cur_mask); + return false; + } + /* + * Once the granule is frozen we can't change the mask anymore. If by + * chance the hotplugged device supports the same granule, we can still + * accept it. + */ + if (viommu->granule_frozen) { + int cur_granule = ctz64(cur_mask); + + if (!(BIT_ULL(cur_granule) & new_mask)) { + error_setg(errp, + "virtio-iommu does not support frozen granule 0x%llx", + BIT_ULL(cur_granule)); + return false; + } + } + return true; +} + static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn, HostIOMMUDevice *hiod, Error **errp) { + ERRP_GUARD(); VirtIOIOMMU *viommu = opaque; HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod); struct hiod_key *new_key; @@ -623,8 +653,26 @@ static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn, hiod->aliased_devfn, host_iova_ranges, errp); if (ret) { - g_list_free_full(host_iova_ranges, g_free); - return false; + goto error; + } + } + if (hiodc->get_page_size_mask) { + uint64_t new_mask = hiodc->get_page_size_mask(hiod); + + if (check_page_size_mask(viommu, new_mask, errp)) { + /* + * The default mask depends on the "granule" property. For example, + * with 4k granule, it is -(4 * KiB). When an assigned device has + * page size restrictions due to the hardware IOMMU configuration, + * apply this restriction to the mask. + */ + trace_virtio_iommu_update_page_size_mask(hiod->name, + viommu->config.page_size_mask, + new_mask); + viommu->config.page_size_mask &= new_mask; + } else { + error_prepend(errp, "%s: ", hiod->name); + goto error; } } @@ -637,6 +685,9 @@ static bool virtio_iommu_set_iommu_device(PCIBus *bus, void *opaque, int devfn, g_list_free_full(host_iova_ranges, g_free); return true; +error: + g_list_free_full(host_iova_ranges, g_free); + return false; } static void diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events index 3cf84e04a7..599d855ff6 100644 --- a/hw/virtio/trace-events +++ b/hw/virtio/trace-events @@ -132,6 +132,7 @@ virtio_iommu_notify_map(const char *name, uint64_t virt_start, uint64_t virt_end virtio_iommu_notify_unmap(const char *name, uint64_t virt_start, uint64_t virt_end) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64 virtio_iommu_remap(const char *name, uint64_t virt_start, uint64_t virt_end, uint64_t phys_start) "mr=%s virt_start=0x%"PRIx64" virt_end=0x%"PRIx64" phys_start=0x%"PRIx64 virtio_iommu_set_page_size_mask(const char *name, uint64_t old, uint64_t new) "mr=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64 +virtio_iommu_update_page_size_mask(const char *name, uint64_t old, uint64_t new) "host iommu device=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64 virtio_iommu_notify_flag_add(const char *name) "add notifier to mr %s" virtio_iommu_notify_flag_del(const char *name) "del notifier from mr %s" virtio_iommu_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn, bool on) "Device %02x:%02x.%x switching address space (iommu enabled=%d)"
Retrieve the Host IOMMU Device page size mask when this latter is set. This allows to get the information much sooner than when relying on IOMMU MR set_page_size_mask() call, whcih happens when the IOMMU MR gets enabled. We introduce check_page_size_mask() helper whose code is inherited from current virtio_iommu_set_page_size_mask() implementation. This callback will be removed in a subsequent patch. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- hw/virtio/virtio-iommu.c | 55 ++++++++++++++++++++++++++++++++++++++-- hw/virtio/trace-events | 1 + 2 files changed, 54 insertions(+), 2 deletions(-)