| Message ID | 20200105164801.26278-4-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | More Hyper-V infrastructure | expand |
On 05.01.2020 17:47, Wei Liu wrote: > Hyper-V's input / output argument must be 8 bytes aligned an not cross > page boundary. The easiest way to satisfy those requirements is to use > percpu page. I'm not sure "easiest" is really true here. Others could consider adding __aligned() attributes as easy or even easier (by being even more transparent to use sites). Could we settle on "One way ..."? > @@ -83,14 +84,33 @@ static void __init setup_hypercall_page(void) > wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); > } > > +static void setup_hypercall_pcpu_arg(void) > +{ > + void *mapping; > + > + mapping = alloc_xenheap_page(); > + if ( !mapping ) > + panic("Failed to allocate hypercall input page for %u\n", "... for CPU%u\n" please. > + smp_processor_id()); > + > + this_cpu(hv_pcpu_input_arg) = mapping; When offlining and then re-onlining a CPU, the prior page will be leaked. > --- a/xen/include/asm-x86/guest/hyperv.h > +++ b/xen/include/asm-x86/guest/hyperv.h > @@ -51,6 +51,8 @@ static inline uint64_t hv_scale_tsc(uint64_t tsc, uint64_t scale, > > #ifdef CONFIG_HYPERV_GUEST > > +#include <xen/percpu.h> > + > #include <asm/guest/hypervisor.h> > > struct ms_hyperv_info { > @@ -63,6 +65,8 @@ struct ms_hyperv_info { > }; > extern struct ms_hyperv_info ms_hyperv; > > +DECLARE_PER_CPU(void *, hv_pcpu_input_arg); Will this really be needed outside of the file that defines it? Also, while looking at this I notice that - despite my earlier comment when giving the respective, sort-of-conditional ack - there are (still) many apparently pointless __packed attributes in hyperv-tlfs.h. Care to comment on this? Jan
On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote: > On 05.01.2020 17:47, Wei Liu wrote: > > Hyper-V's input / output argument must be 8 bytes aligned an not cross > > page boundary. The easiest way to satisfy those requirements is to use > > percpu page. > > I'm not sure "easiest" is really true here. Others could consider adding > __aligned() attributes as easy or even easier (by being even more > transparent to use sites). Could we settle on "One way ..."? Do you mean something like struct foo __aligned(8); hv_do_hypercall(OP, virt_to_maddr(&foo), ...); ? I don't think this is transparent to user sites. Plus, foo is on stack which is 1) difficult to get its maddr, 2) may cross page boundary. If I misunderstood what you meant, please give me an example here. > > > @@ -83,14 +84,33 @@ static void __init setup_hypercall_page(void) > > wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); > > } > > > > +static void setup_hypercall_pcpu_arg(void) > > +{ > > + void *mapping; > > + > > + mapping = alloc_xenheap_page(); > > + if ( !mapping ) > > + panic("Failed to allocate hypercall input page for %u\n", > > "... for CPU%u\n" please. > > > + smp_processor_id()); > > + > > + this_cpu(hv_pcpu_input_arg) = mapping; > > When offlining and then re-onlining a CPU, the prior page will be > leaked. Right. Thanks for catching this one. > > > --- a/xen/include/asm-x86/guest/hyperv.h > > +++ b/xen/include/asm-x86/guest/hyperv.h > > @@ -51,6 +51,8 @@ static inline uint64_t hv_scale_tsc(uint64_t tsc, uint64_t scale, > > > > #ifdef CONFIG_HYPERV_GUEST > > > > +#include <xen/percpu.h> > > + > > #include <asm/guest/hypervisor.h> > > > > struct ms_hyperv_info { > > @@ -63,6 +65,8 @@ struct ms_hyperv_info { > > }; > > extern struct ms_hyperv_info ms_hyperv; > > > > +DECLARE_PER_CPU(void *, hv_pcpu_input_arg); > > Will this really be needed outside of the file that defines it? > This can live in a private header for the time being. > Also, while looking at this I notice that - despite my earlier > comment when giving the respective, sort-of-conditional ack - > there are (still) many apparently pointless __packed attributes > in hyperv-tlfs.h. Care to comment on this? Again, that's a straight import from Linux. I tried not to deviate too much. A commit in Linux (ec084491727b0) claims "compiler can add alignment padding to structures or reorder struct members for randomization and optimization". I just checked all the packed structures. They seem to have all the required manual paddings already. I can only assume they tried to erred on the safe side. Wei. > > Jan
From: Wei Liu <wl@xen.org> Sent: Tuesday, January 7, 2020 8:34 AM > > On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote: > > On 05.01.2020 17:47, Wei Liu wrote: > > > Hyper-V's input / output argument must be 8 bytes aligned an not cross > > > page boundary. The easiest way to satisfy those requirements is to use > > > percpu page. > > > > I'm not sure "easiest" is really true here. Others could consider adding > > __aligned() attributes as easy or even easier (by being even more > > transparent to use sites). Could we settle on "One way ..."? > > Do you mean something like > > struct foo __aligned(8); > > hv_do_hypercall(OP, virt_to_maddr(&foo), ...); > > ? > > I don't think this is transparent to user sites. Plus, foo is on stack > which is 1) difficult to get its maddr, 2) may cross page boundary. > > If I misunderstood what you meant, please give me an example here. > > > > > > @@ -83,14 +84,33 @@ static void __init setup_hypercall_page(void) > > > wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); > > > } > > > > > > +static void setup_hypercall_pcpu_arg(void) > > > +{ > > > + void *mapping; > > > + > > > + mapping = alloc_xenheap_page(); > > > + if ( !mapping ) > > > + panic("Failed to allocate hypercall input page for %u\n", > > > > "... for CPU%u\n" please. > > > > > + smp_processor_id()); > > > + > > > + this_cpu(hv_pcpu_input_arg) = mapping; > > > > When offlining and then re-onlining a CPU, the prior page will be > > leaked. > > Right. Thanks for catching this one. > > > > > > --- a/xen/include/asm-x86/guest/hyperv.h > > > +++ b/xen/include/asm-x86/guest/hyperv.h > > > @@ -51,6 +51,8 @@ static inline uint64_t hv_scale_tsc(uint64_t tsc, uint64_t scale, > > > > > > #ifdef CONFIG_HYPERV_GUEST > > > > > > +#include <xen/percpu.h> > > > + > > > #include <asm/guest/hypervisor.h> > > > > > > struct ms_hyperv_info { > > > @@ -63,6 +65,8 @@ struct ms_hyperv_info { > > > }; > > > extern struct ms_hyperv_info ms_hyperv; > > > > > > +DECLARE_PER_CPU(void *, hv_pcpu_input_arg); > > > > Will this really be needed outside of the file that defines it? > > > > This can live in a private header for the time being. > > > Also, while looking at this I notice that - despite my earlier > > comment when giving the respective, sort-of-conditional ack - > > there are (still) many apparently pointless __packed attributes > > in hyperv-tlfs.h. Care to comment on this? > > Again, that's a straight import from Linux. I tried not to deviate too > much. A commit in Linux (ec084491727b0) claims "compiler can add > alignment padding to structures or reorder struct members for > randomization and optimization". > > I just checked all the packed structures. They seem to have all the > required manual paddings already. I can only assume they tried to erred > on the safe side. Correct. The __packed attribute was added only about a year ago after somebody on LKML noticed that the structures were not packed. Some discussion ensued, but the consensus was to add __packed due to general paranoia about what the compiler might do even though individual fields are aligned to their natural boundary. Michael > > Wei. > > > > > Jan
On 07.01.2020 17:33, Wei Liu wrote: > On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote: >> On 05.01.2020 17:47, Wei Liu wrote: >>> Hyper-V's input / output argument must be 8 bytes aligned an not cross >>> page boundary. The easiest way to satisfy those requirements is to use >>> percpu page. >> >> I'm not sure "easiest" is really true here. Others could consider adding >> __aligned() attributes as easy or even easier (by being even more >> transparent to use sites). Could we settle on "One way ..."? > > Do you mean something like > > struct foo __aligned(8); If this is in a header and ... > hv_do_hypercall(OP, virt_to_maddr(&foo), ...); ... this in actual code, then yes. > ? > > I don't think this is transparent to user sites. Plus, foo is on stack > which is 1) difficult to get its maddr, It being on the stack may indeed complicate getting its machine address (if not now, then down the road) - valid point. > 2) may cross page boundary. The __aligned() of course needs to be large enough to avoid this happening. >> Also, while looking at this I notice that - despite my earlier >> comment when giving the respective, sort-of-conditional ack - >> there are (still) many apparently pointless __packed attributes >> in hyperv-tlfs.h. Care to comment on this? > > Again, that's a straight import from Linux. I tried not to deviate too > much. A commit in Linux (ec084491727b0) claims "compiler can add > alignment padding to structures or reorder struct members for > randomization and optimization". Would a compiler doing so (without explicitly being told to) even be in line with the C spec? I'd buy such a claim only if I see an example proving it. > I just checked all the packed structures. They seem to have all the > required manual paddings already. I can only assume they tried to erred > on the safe side. And you surely recall we had to remove quite a few instances of __packed for gcc 9 compatibility? Jan
On Tue, Jan 07, 2020 at 06:08:19PM +0100, Jan Beulich wrote: > On 07.01.2020 17:33, Wei Liu wrote: > > On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote: > >> On 05.01.2020 17:47, Wei Liu wrote: > >>> Hyper-V's input / output argument must be 8 bytes aligned an not cross > >>> page boundary. The easiest way to satisfy those requirements is to use > >>> percpu page. > >> > >> I'm not sure "easiest" is really true here. Others could consider adding > >> __aligned() attributes as easy or even easier (by being even more > >> transparent to use sites). Could we settle on "One way ..."? > > > > Do you mean something like > > > > struct foo __aligned(8); > > If this is in a header and ... > > > hv_do_hypercall(OP, virt_to_maddr(&foo), ...); > > ... this in actual code, then yes. > > > ? > > > > I don't think this is transparent to user sites. Plus, foo is on stack > > which is 1) difficult to get its maddr, > > It being on the stack may indeed complicate getting its machine address > (if not now, then down the road) - valid point. > > > 2) may cross page boundary. > > The __aligned() of course needs to be large enough to avoid this > happening. For this alignment to be large enough, it will need to be of PAGE_SIZE, right? Wouldn't that blow up Xen's stack easily? Given we only have two pages for that. In light of these restrictions, the approach I take in the original patch should be okay. I'm fine with changing the wording to "One way ..." -- if that's the only objection you have after this mail. > > >> Also, while looking at this I notice that - despite my earlier > >> comment when giving the respective, sort-of-conditional ack - > >> there are (still) many apparently pointless __packed attributes > >> in hyperv-tlfs.h. Care to comment on this? > > > > Again, that's a straight import from Linux. I tried not to deviate too > > much. A commit in Linux (ec084491727b0) claims "compiler can add > > alignment padding to structures or reorder struct members for > > randomization and optimization". > > Would a compiler doing so (without explicitly being told to) even > be in line with the C spec? I'd buy such a claim only if I see an > example proving it. > > > I just checked all the packed structures. They seem to have all the > > required manual paddings already. I can only assume they tried to erred > > on the safe side. > > And you surely recall we had to remove quite a few instances of > __packed for gcc 9 compatibility? Fair enough. I will write a patch to drop those __packed attributes. Wei. > > Jan
On 07.01.2020 18:27, Wei Liu wrote: > On Tue, Jan 07, 2020 at 06:08:19PM +0100, Jan Beulich wrote: >> On 07.01.2020 17:33, Wei Liu wrote: >>> On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote: >>>> On 05.01.2020 17:47, Wei Liu wrote: >>>>> Hyper-V's input / output argument must be 8 bytes aligned an not cross >>>>> page boundary. The easiest way to satisfy those requirements is to use >>>>> percpu page. >>>> >>>> I'm not sure "easiest" is really true here. Others could consider adding >>>> __aligned() attributes as easy or even easier (by being even more >>>> transparent to use sites). Could we settle on "One way ..."? >>> >>> Do you mean something like >>> >>> struct foo __aligned(8); >> >> If this is in a header and ... >> >>> hv_do_hypercall(OP, virt_to_maddr(&foo), ...); >> >> ... this in actual code, then yes. >> >>> ? >>> >>> I don't think this is transparent to user sites. Plus, foo is on stack >>> which is 1) difficult to get its maddr, >> >> It being on the stack may indeed complicate getting its machine address >> (if not now, then down the road) - valid point. >> >>> 2) may cross page boundary. >> >> The __aligned() of course needs to be large enough to avoid this >> happening. > > For this alignment to be large enough, it will need to be of PAGE_SIZE, > right? Wouldn't that blow up Xen's stack easily? Given we only have two > pages for that. Why PAGE_SIZE? For example, a 24-byte structure won't cross a page boundary if aligned to 32 bytes. > In light of these restrictions, the approach I take in the original > patch should be okay. > > I'm fine with changing the wording to "One way ..." -- if that's the > only objection you have after this mail. Well, the goal was to (a) check whether alternatives have been considered (and if not, to consider them) and then (b) if we stick to your approach, slightly change the wording as suggested. Jan
On 07.01.2020 17:45, Michael Kelley wrote: > From: Wei Liu <wl@xen.org> Sent: Tuesday, January 7, 2020 8:34 AM >> >> On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote: >>> On 05.01.2020 17:47, Wei Liu wrote: >>>> Hyper-V's input / output argument must be 8 bytes aligned an not cross >>>> page boundary. The easiest way to satisfy those requirements is to use >>>> percpu page. >>> >>> I'm not sure "easiest" is really true here. Others could consider adding >>> __aligned() attributes as easy or even easier (by being even more >>> transparent to use sites). Could we settle on "One way ..."? >> >> Do you mean something like >> >> struct foo __aligned(8); >> >> hv_do_hypercall(OP, virt_to_maddr(&foo), ...); >> >> ? >> >> I don't think this is transparent to user sites. Plus, foo is on stack >> which is 1) difficult to get its maddr, 2) may cross page boundary. >> >> If I misunderstood what you meant, please give me an example here. >> >>> >>>> @@ -83,14 +84,33 @@ static void __init setup_hypercall_page(void) >>>> wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); >>>> } >>>> >>>> +static void setup_hypercall_pcpu_arg(void) >>>> +{ >>>> + void *mapping; >>>> + >>>> + mapping = alloc_xenheap_page(); >>>> + if ( !mapping ) >>>> + panic("Failed to allocate hypercall input page for %u\n", >>> >>> "... for CPU%u\n" please. >>> >>>> + smp_processor_id()); >>>> + >>>> + this_cpu(hv_pcpu_input_arg) = mapping; >>> >>> When offlining and then re-onlining a CPU, the prior page will be >>> leaked. >> >> Right. Thanks for catching this one. >> >>> >>>> --- a/xen/include/asm-x86/guest/hyperv.h >>>> +++ b/xen/include/asm-x86/guest/hyperv.h >>>> @@ -51,6 +51,8 @@ static inline uint64_t hv_scale_tsc(uint64_t tsc, uint64_t scale, >>>> >>>> #ifdef CONFIG_HYPERV_GUEST >>>> >>>> +#include <xen/percpu.h> >>>> + >>>> #include <asm/guest/hypervisor.h> >>>> >>>> struct ms_hyperv_info { >>>> @@ -63,6 +65,8 @@ struct ms_hyperv_info { >>>> }; >>>> extern struct ms_hyperv_info ms_hyperv; >>>> >>>> +DECLARE_PER_CPU(void *, hv_pcpu_input_arg); >>> >>> Will this really be needed outside of the file that defines it? >>> >> >> This can live in a private header for the time being. >> >>> Also, while looking at this I notice that - despite my earlier >>> comment when giving the respective, sort-of-conditional ack - >>> there are (still) many apparently pointless __packed attributes >>> in hyperv-tlfs.h. Care to comment on this? >> >> Again, that's a straight import from Linux. I tried not to deviate too >> much. A commit in Linux (ec084491727b0) claims "compiler can add >> alignment padding to structures or reorder struct members for >> randomization and optimization". >> >> I just checked all the packed structures. They seem to have all the >> required manual paddings already. I can only assume they tried to erred >> on the safe side. > > Correct. The __packed attribute was added only about a year ago > after somebody on LKML noticed that the structures were not packed. > Some discussion ensued, but the consensus was to add __packed due > to general paranoia about what the compiler might do even though > individual fields are aligned to their natural boundary. Which, as wwe've found the hard way, then leads to problems (with at least gcc 9) when wanting to take the address of a member of such a structure, as the compiler then (mostly validly) assumes the pointer won't be naturally aligned. Hence, as also said in reply to Wei elsewhere, we found it necessary to drop various __packed in order to be able to build Xen with gcc 9. Jan
On Wed, Jan 08, 2020 at 11:55:03AM +0100, Jan Beulich wrote: > On 07.01.2020 18:27, Wei Liu wrote: > > On Tue, Jan 07, 2020 at 06:08:19PM +0100, Jan Beulich wrote: > >> On 07.01.2020 17:33, Wei Liu wrote: > >>> On Mon, Jan 06, 2020 at 11:27:18AM +0100, Jan Beulich wrote: > >>>> On 05.01.2020 17:47, Wei Liu wrote: > >>>>> Hyper-V's input / output argument must be 8 bytes aligned an not cross > >>>>> page boundary. The easiest way to satisfy those requirements is to use > >>>>> percpu page. > >>>> > >>>> I'm not sure "easiest" is really true here. Others could consider adding > >>>> __aligned() attributes as easy or even easier (by being even more > >>>> transparent to use sites). Could we settle on "One way ..."? > >>> > >>> Do you mean something like > >>> > >>> struct foo __aligned(8); > >> > >> If this is in a header and ... > >> > >>> hv_do_hypercall(OP, virt_to_maddr(&foo), ...); > >> > >> ... this in actual code, then yes. > >> > >>> ? > >>> > >>> I don't think this is transparent to user sites. Plus, foo is on stack > >>> which is 1) difficult to get its maddr, > >> > >> It being on the stack may indeed complicate getting its machine address > >> (if not now, then down the road) - valid point. > >> > >>> 2) may cross page boundary. > >> > >> The __aligned() of course needs to be large enough to avoid this > >> happening. > > > > For this alignment to be large enough, it will need to be of PAGE_SIZE, > > right? Wouldn't that blow up Xen's stack easily? Given we only have two > > pages for that. > > Why PAGE_SIZE? For example, a 24-byte structure won't cross a page > boundary if aligned to 32 bytes. > You're right. I said PAGE_SIZE because I was too lazy to calculate the size of every structures. That's tedious and error prone. > > In light of these restrictions, the approach I take in the original > > patch should be okay. > > > > I'm fine with changing the wording to "One way ..." -- if that's the > > only objection you have after this mail. > > Well, the goal was to (a) check whether alternatives have been considered > (and if not, to consider them) and then (b) if we stick to your approach, > slightly change the wording as suggested. I think the determining factor here is to the difficulty of getting maddr of a stack variable. I will stick with this approach and change the wording. Wei. > > Jan
diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c index 381be2a68c..7e046dfc04 100644 --- a/xen/arch/x86/guest/hyperv/hyperv.c +++ b/xen/arch/x86/guest/hyperv/hyperv.c @@ -27,6 +27,7 @@ struct ms_hyperv_info __read_mostly ms_hyperv; extern char hv_hypercall_page[]; +DEFINE_PER_CPU_READ_MOSTLY(void *, hv_pcpu_input_arg); static const struct hypervisor_ops ops; const struct hypervisor_ops *__init hyperv_probe(void) @@ -83,14 +84,33 @@ static void __init setup_hypercall_page(void) wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64); } +static void setup_hypercall_pcpu_arg(void) +{ + void *mapping; + + mapping = alloc_xenheap_page(); + if ( !mapping ) + panic("Failed to allocate hypercall input page for %u\n", + smp_processor_id()); + + this_cpu(hv_pcpu_input_arg) = mapping; +} + static void __init setup(void) { setup_hypercall_page(); + setup_hypercall_pcpu_arg(); +} + +static void ap_setup(void) +{ + setup_hypercall_pcpu_arg(); } static const struct hypervisor_ops ops = { .name = "Hyper-V", .setup = setup, + .ap_setup = ap_setup, }; /* diff --git a/xen/include/asm-x86/guest/hyperv.h b/xen/include/asm-x86/guest/hyperv.h index c7a7f32bd5..6cf2eab62f 100644 --- a/xen/include/asm-x86/guest/hyperv.h +++ b/xen/include/asm-x86/guest/hyperv.h @@ -51,6 +51,8 @@ static inline uint64_t hv_scale_tsc(uint64_t tsc, uint64_t scale, #ifdef CONFIG_HYPERV_GUEST +#include <xen/percpu.h> + #include <asm/guest/hypervisor.h> struct ms_hyperv_info { @@ -63,6 +65,8 @@ struct ms_hyperv_info { }; extern struct ms_hyperv_info ms_hyperv; +DECLARE_PER_CPU(void *, hv_pcpu_input_arg); + const struct hypervisor_ops *hyperv_probe(void); #else
Hyper-V's input / output argument must be 8 bytes aligned an not cross page boundary. The easiest way to satisfy those requirements is to use percpu page. For the foreseeable future we only need to provide input for TLB and APIC hypercalls, so skip setting up an output page. We will also need to provide an ap_setup hook for secondary cpus to setup its own input page. Signed-off-by: Wei Liu <liuwe@microsoft.com> --- v3: 1. Use xenheap page instead 2. Drop page tracking structure 3. Drop Paul's review tag --- xen/arch/x86/guest/hyperv/hyperv.c | 20 ++++++++++++++++++++ xen/include/asm-x86/guest/hyperv.h | 4 ++++ 2 files changed, 24 insertions(+)