On Tue, Dec 19, 2023 at 05:06:32PM +0100, Jan Beulich wrote:
> On 15.12.2023 15:18, Roger Pau Monne wrote:
> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -370,10 +370,88 @@ static unsigned int __hwdom_init
> > hwdom_iommu_map(const struct domain *d,
> > return perms;
> > }
> >
> > +struct map_data {
> > + struct domain *d;
> > + unsigned int flush_flags;
> > + bool mmio_ro;
> > +};
> > +
> > +static int __hwdom_init cf_check identity_map(unsigned long s, unsigned
> > long e,
> > + void *data)
> > +{
> > + struct map_data *info = data;
> > + struct domain *d = info->d;
> > + long rc;
> > +
> > + if ( iommu_verbose )
> > + printk(XENLOG_INFO " [%010lx, %010lx] R%c\n",
> > + s, e, info->mmio_ro ? 'O' : 'W');
> > +
> > + if ( paging_mode_translate(d) )
> > + {
> > + if ( info->mmio_ro )
> > + {
> > + ASSERT_UNREACHABLE();
> > + return 0;
>
> Is this meant to be in line with the main return statement's comment?
> I'm inclined to ask for an actual error code (-EOPNOTSUPP?) here.
Hm, yes, for that one it might make sense to return -EOPNOTSUPP, as
all attempts to map will end up failing anyway.
> > + }
> > + while ( (rc = map_mmio_regions(d, _gfn(s), e - s + 1, _mfn(s))) >
> > 0 )
> > + {
> > + s += rc;
> > + process_pending_softirqs();
> > + }
> > + }
> > + else
> > + {
> > + const unsigned int perms = IOMMUF_readable | IOMMUF_preempt |
> > + (info->mmio_ro ? 0 : IOMMUF_writable);
> > +
> > + /*
> > + * Read-only ranges are only created based on the contents of mmio
> > + * read-only rangeset, and hence need the additional iomem
> > permissions
> > + * check.
> > + */
> > + while( info->mmio_ro && s <= e && !iomem_access_permitted(d, s, e)
> > )
>
> Nit: Missing blank after "while".
>
> > + {
> > + /*
> > + * Consume a frame per iteration until the reminder is
> > accessible
>
> Nit: remainder?
>
> > + * or there's nothing left to map.
> > + */
> > + if ( iomem_access_permitted(d, s, s) )
> > + {
> > + rc = iommu_map(d, _dfn(s), _mfn(s), 1, perms,
> > + &info->flush_flags);
> > + if ( rc < 0 )
> > + break;
> > + /* Must map a frame at least, which is what we request
> > for. */
> > + ASSERT(rc == 1);
> > + process_pending_softirqs();
> > + }
> > + s++;
> > + }
> > + while ( (rc = iommu_map(d, _dfn(s), _mfn(s), e - s + 1,
> > + perms, &info->flush_flags)) > 0 )
> > + {
> > + s += rc;
> > + process_pending_softirqs();
> > + }
> > + }
> > + ASSERT(rc <= 0);
> > + if ( rc )
> > + printk(XENLOG_WARNING
> > + "IOMMU identity mapping of [%lx, %lx] failed: %ld\n",
> > + s, e, rc);
> > +
> > + /* Ignore errors and attempt to map the remaining regions. */
> > + return 0;
> > +}
> > +
> > void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> > {
> > unsigned long i, top, max_pfn, start, count;
> > unsigned int flush_flags = 0, start_perms = 0;
> > + struct rangeset *map;
> > + struct map_data map_data = { .d = d };
> > + int rc;
> >
> > BUG_ON(!is_hardware_domain(d));
> >
> > @@ -397,6 +475,10 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain
> > *d)
> > if ( iommu_hwdom_passthrough )
> > return;
> >
> > + map = rangeset_new(NULL, NULL, 0);
> > + if ( !map )
> > + panic("IOMMU init: unable to allocate rangeset\n");
> > +
> > max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
> > top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
> >
> > @@ -451,8 +533,26 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain
> > *d)
> > goto commit;
> > }
> >
> > + if ( iommu_verbose )
> > + printk(XENLOG_INFO "%pd: identity mappings for IOMMU:\n", d);
> > +
> > + rc = rangeset_report_ranges(map, 0, ~0UL, identity_map, &map_data);
> > + if ( rc )
> > + panic("IOMMU unable to create mappings: %d\n", rc);
> > + rangeset_destroy(map);
> > +
> > + if ( is_pv_domain(d) )
> > + {
> > + map_data.mmio_ro = true;
> > + rc = rangeset_report_ranges(mmio_ro_ranges, 0, ~0UL, identity_map,
> > + &map_data);
> > + if ( rc )
> > + panic("IOMMU unable to create read-only mappings: %d\n", rc);
> > + }
>
> As it stands identity_map() deliberately returns no error. Yet here
> you panic() in case of receiving an error, despite that being impossible?
I wasn't sure whether rangeset_report_ranges() itself could return an
error. Thinking twice, we might want to print a message and attempt
to continue, as in the worst case dom0 might be missing maps.
> Also if we want/need to panic() here, can we avoid having two instances
> of almost the same string literal in .rodata? Along the lines of
>
> rc = rangeset_report_ranges(map, 0, ~0UL, identity_map, &map_data);
> rangeset_destroy(map);
> if ( !rc && is_pv_domain(d) )
> {
> map_data.mmio_ro = true;
> rc = rangeset_report_ranges(mmio_ro_ranges, 0, ~0UL, identity_map,
> &map_data);
> }
> if ( rc )
> panic("IOMMU unable to create %smappings: %d\n",
> map_data.mmio_ro ? "read-only " : "", rc);
>
> ?
>
> > + map_data.flush_flags |= flush_flags;
>
> So you decided to still keep the standalone "flush_flags" around. Is
> there a particular reason?
Oh, git distracted with the other changes and forgot about this one.
Thanks, Roger.
