On Mon, Oct 17, 2022 at 11:13 PM Peter Xu <pet...@redhat.com> wrote:
>
> On Mon, Oct 17, 2022 at 03:52:46PM +0800, Jason Wang wrote:
>
> [...]
>
> > > > +struct vtd_iotlb_key {
> > > > + uint16_t sid;
> > > > + uint32_t pasid;
> > > > + uint64_t gfn;
> > > > + uint32_t level;
> > > > };
> > >
> > > Nit: maybe re-arrange it a bit?
> > >
> > > struct vtd_iotlb_key {
> > > uint64_t gfn;
> > > uint32_t pasid;
> > > uint32_t level;
> > > uint16_t sid;
> > > } __attribute__((__packed__));
> > >
> > > "packed" should save us 6 bytes for each in this case, maybe also
> > > worthwhile but not strongly as we have a limit of 1k objs.
> >
> > I think it should be fine to rearrange but for 'packed', would this
> > cause alignment issues that may cause troubles on some arches?
>
> Do you mean the gfn reading can be split into 2 * 4 bytes?
Somehow, e.g the gfn is not at the 8-byte boundary which may result in
non-aligned access. Which will trigger an exception on some arch
(arm).
> Would that
> still work as long as we're protected with a lock when accessing iotlb
> (even though it may be slower than aligned access)?
Probably not, since we have code to access the full uint64_t gfn.
>
> But at least I think you're right it's not always a benefit, so no strong
> opinion here to have it packed.
Ok, so I will not use packed in the next version.
>
> >
> > >
> > > The name "gfn" seems a bit unfortunate - would "iova" be more suitable? I
> > > do see we used it elsewhere too, so we can also leave that for later.
> >
> > Right, it has been used for VTDIOTLBEntry before this patch. If
> > possible I would leave it to be done on top with a separate patch.
>
> Definitely.
>
> Thanks,
Thanks
>
> --
> Peter Xu
>
