Re: [PATCH] Add support for ESRT loading under Xen

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Sep 06, 2022 at 08:49:54AM +0200, Jan Beulich wrote:
> On 26.08.2022 20:01, Demi Marie Obenour wrote:
> > On Fri, Aug 26, 2022 at 09:53:29AM +0200, Jan Beulich wrote:
> >> On 25.08.2022 23:52, Demi Marie Obenour wrote:
> >>> @@ -40,6 +41,38 @@
> >>>  
> >>>  #define efi_data(op)     (op.u.efi_runtime_call)
> >>>  
> >>> +static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> >>> +              "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> >>> +
> >>> +bool xen_efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *md)
> >>> +{
> >>> + struct xen_platform_op op = {
> >>> +         .cmd = XENPF_firmware_info,
> >>> +         .u.firmware_info = {
> >>> +                 .type = XEN_FW_EFI_INFO,
> >>> +                 .index = XEN_FW_EFI_MEM_INFO,
> >>> +                 .u.efi_info.mem.addr = phys_addr,
> >>> +                 .u.efi_info.mem.size = ((u64)-1ULL) - phys_addr,
> >>> +         }
> >>> + };
> >>> + union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> >>> + int rc;
> >>> +
> >>> + memset(md, 0, sizeof(*md)); /* initialize md even on failure */
> >>> + rc = HYPERVISOR_platform_op(&op);
> >>> + if (rc) {
> >>> +         pr_warn("Could not obtain information on address %llu from Xen: 
> >>> "
> >>> +                 "error %d\n", phys_addr, rc);
> >>> +         return false;
> >>> + }
> >>> +
> >>> + md->attribute = info->mem.attr;
> >>> + md->type = info->mem.type;
> >>> + md->num_pages = info->mem.size >> XEN_PAGE_SHIFT;
> >>> + md->phys_addr = info->mem.addr;
> >>
> >> As indicated in reply to your patch changing XEN_FW_EFI_MEM_INFO in
> >> the hypervisor: While this may fit the ESRT purpose, the address you
> >> return here is not necessarily the start of the region, and hence
> >> this function is not a general Xen replacement for the non-Xen
> >> function. Therefore I think it also shouldn't give the impression of
> >> doing so.
> > 
> > Is this just a matter of renaming the function?
> 
> Besides renaming the function perhaps it also shouldn't give the
> impression of being generally usable. I would expect it to be a static
> helper somewhere, or even be expanded inline.

I would be fine with doing this, but I didn’t want to litter esrt.c with
Xen-specific code.  IIUC Linux prefers to avoid #ifdef in .c files.

> >  Is it possible to
> > implement the original function with the current hypervisor?
> 
> Yes, but doing so would be ugly: You'd need to "bisect" your way to
> the start of the region.
> 
> As an aside (I think I did point this out before): Can you please
> adjust the way your mail program sends mails? When I respond to your
> mail (using Thunderbird), I find all the people previously on Cc on
> the To: list, while your address is lost. As indicated I believe
> this is a result of the Mail-Followup-To: tag your reply came with
> (and I further think that TB's treatment of that tag is a reasonable
> one, albeit perhaps there are other reasonable treatments as well; I
> am not aware of this tag having any formally specified treatment).

This was a misconfiguration on my end: I marked xen-devel as subscribed
in my muttrc.  I fixed this and also unset followup_to, so the
Mail-Followup-To header should no longer be generated.  Please let me
know if this is still a problem.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature