linux-kernel - Re: [PATCH 1/3] KVM: Support sharing gpc locks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2d421cb18dfa1b88e5025f2f9b94e146c0858960.camel@infradead.org>
Date:   Sun, 29 Jan 2023 11:08:51 +0100
From:   David Woodhouse <dwmw2@...radead.org>
To:     David Stevens <stevensd@...omium.org>,
        Sean Christopherson <seanjc@...gle.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] KVM: Support sharing gpc locks

(Resending as the corporate email system has taken to adding redundant
information in transit as an HTML part — to a message that didn't need
it, which already had it in the Organization: header anyway, and was
previously plain text only.)

On Fri, 2023-01-27 at 13:44 +0900, David Stevens wrote:
> @@ -316,19 +316,19 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
>                  * takes them more than one at a time. Set a subclass on the
>                  * gpc1 lock to make lockdep shut up about it.
>                  */
> -               lock_set_subclass(&gpc1->lock.dep_map, 1, _THIS_IP_);
> +               lock_set_subclass(gpc1->lock.dep_map, 1, _THIS_IP_);

The test robot already pointed out this should be gpc1->lock->dep_map,
but if we do what I said below in the first attempt at this email, this
all goes away anyway.


>                 if (atomic) {
> -                       if (!read_trylock(&gpc2->lock)) {
> -                               read_unlock_irqrestore(&gpc1->lock, flags);
> +                       if (!read_trylock(gpc2->lock)) {
> +                               read_unlock_irqrestore(gpc1->lock, flags);
>                                 return;
>                         }

LGTM without having had time to go through it in detail. As Sean said,
we were waiting for someone (perhaps even ourselves) to do this.
Thanks.

For the runstate one above, this is crying out for gpc1 and gpc2 to
*share* a lock, and ditch the relocking and the whole comment about
being able to declare a lock ordering between the two, which is half
shown in the context cited above.

Probably worth doing that in a separate patch on top of this; I'll take
a look at it when my feet next hit the ground on Tuesday if you haven't
already done so. It's easy to test with the xen_shinfo_test self-test.

Or with this if you want more fun:
https://lore.kernel.org/qemu-devel/20230128081113.1615111-1-dwmw2@infradead.org/

:)




Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)