From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1239CC71155 for ; Wed, 2 Dec 2020 08:00:53 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9166D20872 for ; Wed, 2 Dec 2020 08:00:52 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9166D20872 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.42472.76379 (Exim 4.92) (envelope-from ) id 1kkN40-0003c2-Mr; Wed, 02 Dec 2020 08:00:36 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 42472.76379; Wed, 02 Dec 2020 08:00:36 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kkN40-0003bv-Jf; Wed, 02 Dec 2020 08:00:36 +0000 Received: by outflank-mailman (input) for mailman id 42472; Wed, 02 Dec 2020 08:00:35 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kkN3z-0003bq-AI for xen-devel@lists.xenproject.org; Wed, 02 Dec 2020 08:00:35 +0000 Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 8e60884d-8b66-4a16-9af0-d577dcd50c5d; Wed, 02 Dec 2020 08:00:34 +0000 (UTC) Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 622F2AC2D; Wed, 2 Dec 2020 08:00:33 +0000 (UTC) X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: 8e60884d-8b66-4a16-9af0-d577dcd50c5d X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1606896033; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=g6FYMsGWpX8KoLJIZ4SxdHBjh5HKNe1VFSZksiQciHU=; b=TgdOqlZCy72ckynxbg33GXrVdLZ+6wPcKZ+pJo2D/k4tYjLqBzIxHmWb/wp7QXABb3Ud0D bpEtnY2oDzWRjmGDGr3DYjzu+yvUJvPZuw7hoodtFZEvbPkPMFohZXn8w1bWwB1NldsSco u6kuDx+diToy7BvMAjgThrIDRmtE6c8= Subject: Re: [PATCH V3 01/23] x86/ioreq: Prepare IOREQ feature for making it common To: Oleksandr Cc: Oleksandr Tyshchenko , Paul Durrant , Andrew Cooper , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= , Wei Liu , Julien Grall , Stefano Stabellini , Julien Grall , xen-devel@lists.xenproject.org, =?UTF-8?Q?Alex_Benn=c3=a9e?= References: <1606732298-22107-1-git-send-email-olekstysh@gmail.com> <1606732298-22107-2-git-send-email-olekstysh@gmail.com> <87eek9u6tj.fsf@linaro.org> From: Jan Beulich Message-ID: <802c49d5-00bb-9e10-70d7-2629913b08c9@suse.com> Date: Wed, 2 Dec 2020 09:00:29 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 01.12.2020 19:53, Oleksandr wrote: > On 01.12.20 13:03, Alex Bennée wrote: >> Oleksandr Tyshchenko writes: >>> @@ -1112,19 +1155,11 @@ int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id, >>> if ( s->emulator != current->domain ) >>> goto out; >>> >>> - rc = p2m_set_ioreq_server(d, flags, s); >>> + rc = arch_ioreq_server_map_mem_type(d, s, flags); >>> >>> out: >>> spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock); >>> >>> - if ( rc == 0 && flags == 0 ) >>> - { >>> - struct p2m_domain *p2m = p2m_get_hostp2m(d); >>> - >>> - if ( read_atomic(&p2m->ioreq.entry_count) ) >>> - p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw); >>> - } >>> - >> It should be noted that p2m holds it's own lock but I'm unfamiliar with >> Xen's locking architecture. Is there anything that prevents another vCPU >> accessing a page that is also being used my ioreq on the first vCPU? > I am not sure that I would be able to provide reasonable explanations here. > All what I understand is that p2m_change_entry_type_global() x86 > specific (we don't have p2m_ioreq_server concept on Arm) and should > remain as such (not exposed to the common code). > IIRC, I raised a question during V2 review whether we could have ioreq > server lock around the call to p2m_change_entry_type_global() and didn't > get objections. Not getting objections doesn't mean much. Personally I don't recall such a question, but this doesn't mean much. The important thing here is that you properly justify this change in the description (I didn't look at this version of the patch as a whole yet, so quite likely you actually do). This is because you need to guarantee that you don't introduce any lock order violations by this. There also should be an attempt to avoid future introduction of issues, by adding lock nesting related comments in suitable places. Again, quite likely you actually do so, and I will notice it once looking at the patch as a whole. All of this said, I think it should be tried hard to avoid introducing this extra lock nesting, if there aren't other places already where the same nesting of locks is in effect. > I may mistake, but looks like the lock being used > in p2m_change_entry_type_global() is yet another lock for protecting > page table operations, so unlikely we could get into the trouble calling > this function with the ioreq server lock held. I'm afraid I don't understand the "yet another" here: The ioreq server lock clearly serves an entirely different purpose. Jan