| Message ID | 9810305d-4b36-4e23-b807-a7a00f0ba6b6@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/HVM: load state checking | expand |
On 16/11/2023 1:46 pm, Jan Beulich wrote: > ... to accompany hvm_read_entry() when actual copying isn't desirable. > This allows to remove open-coded stream accesses from hpet_load(), > along with using the helper in hvm_load() itself. > > Since arch_hvm_load()'s declaration would need changing, and since the > function is not used from elsewhere, purge the declaration. With that it > makes little sense to keep arch_hvm_save()'s around; convert that > function to static then at the same time. > > In hpet_load() simplify the specific case of error return that's in > context anyway: There's no need to hold the lock when only updating a > local variable. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> I really do hate all of this logic and most of it wants to live in userspace, but this an improvement. The only thing I'm a little concerned with is the name. hvm_read_entry() clearly consumes an entry, while hvm_point_entry() does, but doesn't obviously convey this at a glance. Ideally I'd say that hvm_{get,copy}_entry() would be better nomeclature, as both have at least a vague implication of unmarshalling and one clearly is making a separate copy. Thoughts? ~Andrew
On 21.11.2023 22:30, Andrew Cooper wrote: > On 16/11/2023 1:46 pm, Jan Beulich wrote: >> ... to accompany hvm_read_entry() when actual copying isn't desirable. >> This allows to remove open-coded stream accesses from hpet_load(), >> along with using the helper in hvm_load() itself. >> >> Since arch_hvm_load()'s declaration would need changing, and since the >> function is not used from elsewhere, purge the declaration. With that it >> makes little sense to keep arch_hvm_save()'s around; convert that >> function to static then at the same time. >> >> In hpet_load() simplify the specific case of error return that's in >> context anyway: There's no need to hold the lock when only updating a >> local variable. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. > I really do hate all of this logic and most of it wants to live in > userspace, but this an improvement. > > The only thing I'm a little concerned with is the name. > hvm_read_entry() clearly consumes an entry, while hvm_point_entry() > does, but doesn't obviously convey this at a glance. > > Ideally I'd say that hvm_{get,copy}_entry() would be better nomeclature, > as both have at least a vague implication of unmarshalling and one > clearly is making a separate copy. I'm fine renaming the new one to hvm_get_entry(). For the existing one, "copy" may be marginally better than "read" / "load", but then it doesn't indicate direction (i.e. somewhat collides with hvm_{write,load}_entry()). So I'd want to leave those as they are. Jan
On 22/11/2023 9:34 am, Jan Beulich wrote: > On 21.11.2023 22:30, Andrew Cooper wrote: >> On 16/11/2023 1:46 pm, Jan Beulich wrote: >>> ... to accompany hvm_read_entry() when actual copying isn't desirable. >>> This allows to remove open-coded stream accesses from hpet_load(), >>> along with using the helper in hvm_load() itself. >>> >>> Since arch_hvm_load()'s declaration would need changing, and since the >>> function is not used from elsewhere, purge the declaration. With that it >>> makes little sense to keep arch_hvm_save()'s around; convert that >>> function to static then at the same time. >>> >>> In hpet_load() simplify the specific case of error return that's in >>> context anyway: There's no need to hold the lock when only updating a >>> local variable. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > Thanks. > >> I really do hate all of this logic and most of it wants to live in >> userspace, but this an improvement. >> >> The only thing I'm a little concerned with is the name. >> hvm_read_entry() clearly consumes an entry, while hvm_point_entry() >> does, but doesn't obviously convey this at a glance. >> >> Ideally I'd say that hvm_{get,copy}_entry() would be better nomeclature, >> as both have at least a vague implication of unmarshalling and one >> clearly is making a separate copy. > I'm fine renaming the new one to hvm_get_entry(). For the existing one, > "copy" may be marginally better than "read" / "load", but then it doesn't > indicate direction (i.e. somewhat collides with hvm_{write,load}_entry()). > So I'd want to leave those as they are. Ok, get/read/write make a reasonably-ok set of names. ~Andrew
--- a/xen/arch/x86/hvm/hpet.c +++ b/xen/arch/x86/hvm/hpet.c @@ -637,7 +637,7 @@ static int cf_check hpet_save(struct vcp static int cf_check hpet_load(struct domain *d, hvm_domain_context_t *h) { HPETState *hp = domain_vhpet(d); - struct hvm_hw_hpet *rec; + const struct hvm_hw_hpet *rec; uint64_t cmp; uint64_t guest_time; int i; @@ -645,17 +645,12 @@ static int cf_check hpet_load(struct dom if ( !has_vhpet(d) ) return -ENODEV; - write_lock(&hp->lock); - /* Reload the HPET registers */ - if ( _hvm_check_entry(h, HVM_SAVE_CODE(HPET), HVM_SAVE_LENGTH(HPET), 1) ) - { - write_unlock(&hp->lock); + rec = hvm_point_entry(HPET, h); + if ( !rec ) return -EINVAL; - } - rec = (struct hvm_hw_hpet *)&h->data[h->cur]; - h->cur += HVM_SAVE_LENGTH(HPET); + write_lock(&hp->lock); #define C(x) hp->hpet.x = rec->x C(capability); --- a/xen/arch/x86/hvm/save.c +++ b/xen/arch/x86/hvm/save.c @@ -15,7 +15,7 @@ #include <public/hvm/save.h> -void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr) +static void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr) { uint32_t eax, ebx, ecx, edx; @@ -30,7 +30,7 @@ void arch_hvm_save(struct domain *d, str d->arch.hvm.sync_tsc = rdtsc(); } -int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr) +static int arch_hvm_load(struct domain *d, const struct hvm_save_header *hdr) { uint32_t eax, ebx, ecx, edx; @@ -277,7 +277,7 @@ int hvm_save(struct domain *d, hvm_domai int hvm_load(struct domain *d, hvm_domain_context_t *h) { - struct hvm_save_header hdr; + const struct hvm_save_header *hdr; struct hvm_save_descriptor *desc; hvm_load_handler handler; struct vcpu *v; @@ -286,11 +286,12 @@ int hvm_load(struct domain *d, hvm_domai if ( d->is_dying ) return -EINVAL; - /* Read the save header, which must be first */ - if ( hvm_load_entry(HEADER, h, &hdr) != 0 ) + /* Get at the save header, which must be first */ + hdr = hvm_point_entry(HEADER, h); + if ( !hdr ) return -ENODATA; - rc = arch_hvm_load(d, &hdr); + rc = arch_hvm_load(d, hdr); if ( rc ) return rc; --- a/xen/arch/x86/include/asm/hvm/save.h +++ b/xen/arch/x86/include/asm/hvm/save.h @@ -39,6 +39,21 @@ void _hvm_write_entry(struct hvm_domain_ int _hvm_check_entry(struct hvm_domain_context *h, uint16_t type, uint32_t len, bool strict_length); +/* + * Unmarshalling: check, then return pointer. Evaluates to non-NULL on success. + * This macro requires the save entry to be the same size as the dest structure. + */ +#define hvm_point_entry(x, h) ({ \ + const void *ptr = NULL; \ + BUILD_BUG_ON(HVM_SAVE_HAS_COMPAT(x)); \ + if ( _hvm_check_entry(h, HVM_SAVE_CODE(x), \ + HVM_SAVE_LENGTH(x), true) == 0 ) \ + { \ + ptr = &(h)->data[(h)->cur]; \ + h->cur += HVM_SAVE_LENGTH(x); \ + } \ + ptr; }) + /* Unmarshalling: copy the contents in a type-safe way */ void _hvm_read_entry(struct hvm_domain_context *h, void *dest, uint32_t dest_len); @@ -127,9 +142,4 @@ int hvm_save_one(struct domain *d, unsig XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz); int hvm_load(struct domain *d, hvm_domain_context_t *h); -/* Arch-specific definitions. */ -struct hvm_save_header; -void arch_hvm_save(struct domain *d, struct hvm_save_header *hdr); -int arch_hvm_load(struct domain *d, struct hvm_save_header *hdr); - #endif /* __XEN_HVM_SAVE_H__ */
... to accompany hvm_read_entry() when actual copying isn't desirable. This allows to remove open-coded stream accesses from hpet_load(), along with using the helper in hvm_load() itself. Since arch_hvm_load()'s declaration would need changing, and since the function is not used from elsewhere, purge the declaration. With that it makes little sense to keep arch_hvm_save()'s around; convert that function to static then at the same time. In hpet_load() simplify the specific case of error return that's in context anyway: There's no need to hold the lock when only updating a local variable. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: New.