| Message ID | 25ad2429-df7d-95c6-ec70-4b23e6a023a2@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IOMMU: superpage support when not sharing pagetables | expand |
On 24/08/2021 15:27, Jan Beulich wrote: > --- a/xen/drivers/passthrough/vtd/utils.c > +++ b/xen/drivers/passthrough/vtd/utils.c > @@ -106,11 +106,6 @@ void print_vtd_entries(struct vtd_iommu > } > > root_entry = (struct root_entry *)map_vtd_domain_page(iommu->root_maddr); Seeing as you're actually cleaning up mapping calls, drop this cast too? Either way, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > - if ( root_entry == NULL ) > - { > - printk(" root_entry == NULL\n"); > - return; > - } > > printk(" root_entry[%02x] = %"PRIx64"\n", bus, root_entry[bus].val); > if ( !root_present(root_entry[bus]) ) >
On 26.08.2021 14:05, Andrew Cooper wrote: > On 24/08/2021 15:27, Jan Beulich wrote: >> --- a/xen/drivers/passthrough/vtd/utils.c >> +++ b/xen/drivers/passthrough/vtd/utils.c >> @@ -106,11 +106,6 @@ void print_vtd_entries(struct vtd_iommu >> } >> >> root_entry = (struct root_entry *)map_vtd_domain_page(iommu->root_maddr); > > Seeing as you're actually cleaning up mapping calls, drop this cast too? There are more of these, so this would be yet another cleanup patch. I didn't get annoyed enough by these to put one together; instead I was hoping that we might touch these lines anyway at some point. E.g. when doing away with the funny map_vtd_domain_page() wrapper around map_domain_page() ... > Either way, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. Jan
Kevin, On 24.08.2021 16:27, Jan Beulich wrote: > map_domain_page() et al never fail; no need to check their return values > against NULL, and no need to carry dead printk()s. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> may I get an ack (or otherwise) on this patch please? Ideally also for some other VT-d specific ones in this series? I'd really like to get in at least some parts of this series, before submitting an even larger v2. Thanks, Jan > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c > @@ -530,12 +530,6 @@ static void amd_dump_page_table_level(st > return; > > table_vaddr = __map_domain_page(pg); > - if ( table_vaddr == NULL ) > - { > - printk("AMD IOMMU failed to map domain page %"PRIpaddr"\n", > - page_to_maddr(pg)); > - return; > - } > > for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ ) > { > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -2800,12 +2800,6 @@ static void vtd_dump_page_table_level(pa > return; > > pt_vaddr = map_vtd_domain_page(pt_maddr); > - if ( pt_vaddr == NULL ) > - { > - printk(VTDPREFIX " failed to map domain page %"PRIpaddr"\n", > - pt_maddr); > - return; > - } > > next_level = level - 1; > for ( i = 0; i < PTE_NUM; i++ ) > --- a/xen/drivers/passthrough/vtd/utils.c > +++ b/xen/drivers/passthrough/vtd/utils.c > @@ -106,11 +106,6 @@ void print_vtd_entries(struct vtd_iommu > } > > root_entry = (struct root_entry *)map_vtd_domain_page(iommu->root_maddr); > - if ( root_entry == NULL ) > - { > - printk(" root_entry == NULL\n"); > - return; > - } > > printk(" root_entry[%02x] = %"PRIx64"\n", bus, root_entry[bus].val); > if ( !root_present(root_entry[bus]) ) > @@ -123,11 +118,6 @@ void print_vtd_entries(struct vtd_iommu > val = root_entry[bus].val; > unmap_vtd_domain_page(root_entry); > ctxt_entry = map_vtd_domain_page(val); > - if ( ctxt_entry == NULL ) > - { > - printk(" ctxt_entry == NULL\n"); > - return; > - } > > val = ctxt_entry[devfn].lo; > printk(" context[%02x] = %"PRIx64"_%"PRIx64"\n", > @@ -151,11 +141,6 @@ void print_vtd_entries(struct vtd_iommu > do > { > l = map_vtd_domain_page(val); > - if ( l == NULL ) > - { > - printk(" l%u == NULL\n", level); > - break; > - } > l_index = get_level_index(gmfn, level); > pte.val = l[l_index]; > unmap_vtd_domain_page(l); > >
> From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, August 24, 2021 10:28 PM > > map_domain_page() et al never fail; no need to check their return values > against NULL, and no need to carry dead printk()s. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c > @@ -530,12 +530,6 @@ static void amd_dump_page_table_level(st > return; > > table_vaddr = __map_domain_page(pg); > - if ( table_vaddr == NULL ) > - { > - printk("AMD IOMMU failed to map domain page %"PRIpaddr"\n", > - page_to_maddr(pg)); > - return; > - } > > for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ ) > { > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -2800,12 +2800,6 @@ static void vtd_dump_page_table_level(pa > return; > > pt_vaddr = map_vtd_domain_page(pt_maddr); > - if ( pt_vaddr == NULL ) > - { > - printk(VTDPREFIX " failed to map domain page %"PRIpaddr"\n", > - pt_maddr); > - return; > - } > > next_level = level - 1; > for ( i = 0; i < PTE_NUM; i++ ) > --- a/xen/drivers/passthrough/vtd/utils.c > +++ b/xen/drivers/passthrough/vtd/utils.c > @@ -106,11 +106,6 @@ void print_vtd_entries(struct vtd_iommu > } > > root_entry = (struct root_entry *)map_vtd_domain_page(iommu- > >root_maddr); > - if ( root_entry == NULL ) > - { > - printk(" root_entry == NULL\n"); > - return; > - } > > printk(" root_entry[%02x] = %"PRIx64"\n", bus, root_entry[bus].val); > if ( !root_present(root_entry[bus]) ) > @@ -123,11 +118,6 @@ void print_vtd_entries(struct vtd_iommu > val = root_entry[bus].val; > unmap_vtd_domain_page(root_entry); > ctxt_entry = map_vtd_domain_page(val); > - if ( ctxt_entry == NULL ) > - { > - printk(" ctxt_entry == NULL\n"); > - return; > - } > > val = ctxt_entry[devfn].lo; > printk(" context[%02x] = %"PRIx64"_%"PRIx64"\n", > @@ -151,11 +141,6 @@ void print_vtd_entries(struct vtd_iommu > do > { > l = map_vtd_domain_page(val); > - if ( l == NULL ) > - { > - printk(" l%u == NULL\n", level); > - break; > - } > l_index = get_level_index(gmfn, level); > pte.val = l[l_index]; > unmap_vtd_domain_page(l);
> From: Jan Beulich <jbeulich@suse.com> > Sent: Wednesday, September 15, 2021 8:42 PM > > Kevin, > > On 24.08.2021 16:27, Jan Beulich wrote: > > map_domain_page() et al never fail; no need to check their return values > > against NULL, and no need to carry dead printk()s. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > may I get an ack (or otherwise) on this patch please? Ideally also > for some other VT-d specific ones in this series? I'd really like > to get in at least some parts of this series, before submitting an > even larger v2. > I haven't found time to check the entire series in detail. Only acked a few ones which look obvious. Thanks Kevin
On 16.09.2021 09:47, Tian, Kevin wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Wednesday, September 15, 2021 8:42 PM >> >> Kevin, >> >> On 24.08.2021 16:27, Jan Beulich wrote: >>> map_domain_page() et al never fail; no need to check their return values >>> against NULL, and no need to carry dead printk()s. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> may I get an ack (or otherwise) on this patch please? Ideally also >> for some other VT-d specific ones in this series? I'd really like >> to get in at least some parts of this series, before submitting an >> even larger v2. > > I haven't found time to check the entire series in detail. Only acked > a few ones which look obvious. That's fine, thanks! It still allows me to move forward a little. Jan
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -530,12 +530,6 @@ static void amd_dump_page_table_level(st return; table_vaddr = __map_domain_page(pg); - if ( table_vaddr == NULL ) - { - printk("AMD IOMMU failed to map domain page %"PRIpaddr"\n", - page_to_maddr(pg)); - return; - } for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ ) { --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -2800,12 +2800,6 @@ static void vtd_dump_page_table_level(pa return; pt_vaddr = map_vtd_domain_page(pt_maddr); - if ( pt_vaddr == NULL ) - { - printk(VTDPREFIX " failed to map domain page %"PRIpaddr"\n", - pt_maddr); - return; - } next_level = level - 1; for ( i = 0; i < PTE_NUM; i++ ) --- a/xen/drivers/passthrough/vtd/utils.c +++ b/xen/drivers/passthrough/vtd/utils.c @@ -106,11 +106,6 @@ void print_vtd_entries(struct vtd_iommu } root_entry = (struct root_entry *)map_vtd_domain_page(iommu->root_maddr); - if ( root_entry == NULL ) - { - printk(" root_entry == NULL\n"); - return; - } printk(" root_entry[%02x] = %"PRIx64"\n", bus, root_entry[bus].val); if ( !root_present(root_entry[bus]) ) @@ -123,11 +118,6 @@ void print_vtd_entries(struct vtd_iommu val = root_entry[bus].val; unmap_vtd_domain_page(root_entry); ctxt_entry = map_vtd_domain_page(val); - if ( ctxt_entry == NULL ) - { - printk(" ctxt_entry == NULL\n"); - return; - } val = ctxt_entry[devfn].lo; printk(" context[%02x] = %"PRIx64"_%"PRIx64"\n", @@ -151,11 +141,6 @@ void print_vtd_entries(struct vtd_iommu do { l = map_vtd_domain_page(val); - if ( l == NULL ) - { - printk(" l%u == NULL\n", level); - break; - } l_index = get_level_index(gmfn, level); pte.val = l[l_index]; unmap_vtd_domain_page(l);
map_domain_page() et al never fail; no need to check their return values against NULL, and no need to carry dead printk()s. Signed-off-by: Jan Beulich <jbeulich@suse.com>